exercise finished #5240
exercise finished #5240
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is functionally correct and handles all the state transformations well, which is great! However, this requires a change because the implementation uses if statements to handle the different action types. A specific requirement for this task is to use a switch statement for this kind of conditional logic [CHECKLIST ITEM #2]. Please refactor your code to use a switch on action.type. This change will also make it straightforward to add a default case to handle unknown actions, which is another requirement [CHECKLIST ITEM #3].
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
src/transformStateWithClones.js
Outdated
| if (action.type === 'clear') { | ||
| nextState = {}; | ||
| } else { | ||
| nextState = { ...currentState }; | ||
| } | ||
|
|
||
| if ( | ||
| action.type === 'removeProperties' && | ||
| Array.isArray(action.keysToRemove) | ||
| ) { | ||
| for (const key of action.keysToRemove) { | ||
| delete nextState[key]; | ||
| } | ||
| } | ||
|
|
||
| if ( | ||
| action.type === 'addProperties' && | ||
| typeof action.extraData === 'object' && | ||
| action.extraData !== null | ||
| ) { | ||
| Object.assign(nextState, action.extraData); | ||
| } |
There was a problem hiding this comment.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job refactoring your code to use switch statements! The core functionality is working correctly, which is excellent.
However, this requires a change because the logic is currently split across two separate switch statements. This makes the code less readable and, more importantly, the second switch block is missing a default case, which is required for proper error handling [CHECKLIST ITEM #3].
Please combine these into a single switch statement to handle all action types (addProperties, removeProperties, and clear). This will also allow you to have one default case to manage any unknown actions.
As a minor cleanup, you can also remove the duplicate module.exports statement at the end of the file. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| switch (action.type) { | ||
| case 'clear': | ||
| nextState = {}; | ||
| break; | ||
|
|
||
| default: | ||
| nextState = { ...currentState }; | ||
| break; | ||
| } | ||
|
|
||
| switch (action.type) { | ||
| case 'removeProperties': | ||
| if (Array.isArray(action.keysToRemove)) { | ||
| for (const key of action.keysToRemove) { | ||
| delete nextState[key]; | ||
| } | ||
| } | ||
| break; | ||
|
|
||
| case 'addProperties': | ||
| if (typeof action.extraData === 'object' && action.extraData !== null) { | ||
| Object.assign(nextState, action.extraData); | ||
| } | ||
| break; | ||
| } |
There was a problem hiding this comment.
While it's good that you've used switch statements, the logic for handling actions is split across two separate blocks. It's better to use a single switch statement to handle all possible action.type values.
This will make the code more readable and standard. Additionally, the second switch statement (starting on line 27) is missing a default case, which violates checklist item #3: 'switch/case should always have default case for error handling'.
|
|
||
| module.exports = transformStateWithClones; | ||
|
|
||
| module.exports = transformStateWithClones; |
There was a problem hiding this comment.
This module.exports statement is a duplicate of the one on line 50 and can be removed.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job refactoring your code into a single switch statement! The logic is much cleaner now.
I am approving this submission as an educational override so you can move forward, but there's one important improvement to make regarding repeated code. Currently, you're cloning the state (nextState = { ...currentState };) inside several case blocks. To follow the "Don't Repeat Yourself" principle [CHECKLIST ITEM #4], this cloning action should be done just once, right before the switch statement begins. This will make your code more efficient and easier to maintain.
Keep up the excellent work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| switch (action.type) { | ||
| case 'clear': | ||
| nextState = {}; | ||
| break; | ||
|
|
||
| case 'addProperties': | ||
| nextState = { ...currentState }; | ||
|
|
||
| if (typeof action.extraData === 'object' && action.extraData !== null) { | ||
| Object.assign(nextState, action.extraData); | ||
| } | ||
| break; | ||
| case 'removeProperties': | ||
| nextState = { ...currentState }; | ||
|
|
||
| if (Array.isArray(action.keysToRemove)) { | ||
| for (const key of action.keysToRemove) { | ||
| delete nextState[key]; | ||
| } | ||
| } | ||
| break; | ||
|
|
||
| default: | ||
| // ação desconhecida → apenas clona o estado | ||
| nextState = { ...currentState }; | ||
| break; | ||
| } |
There was a problem hiding this comment.
This switch statement repeats the state cloning logic (nextState = { ...currentState };) in the addProperties, removeProperties, and default cases. This goes against the "Don't Repeat Yourself" principle mentioned in checklist item #4.
A better approach would be to perform the common action (cloning the state) once, before the switch. The switch can then handle only the unique logic for each action type.
2 participants
No description provided.